Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove extraneous reassignments in output #166

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

bcaller
Copy link
Collaborator

@bcaller bcaller commented Aug 15, 2018

The output should consist of the path from the source to the sink.

Anything which happens after the source reaches the sink is irrelevant
and just makes the output longer and confusing to interpret.

None of the lines removed from the tests actually affected the
vulnerability chain.

Perhaps this should be dealt with somewhere in the definition_chain or
vulnerability functions: here we just trim the chain upon reaching the
sink in the vulnerability_helper.

The output should consist of the path from the source to the sink.

Anything which happens after the source reaches the sink is irrelevant
and just makes the output longer and confusing to interpret.

None of the lines removed from the tests actually affected the
vulnerability chain.

Perhaps this should be dealt with somewhere in the definition_chain or
vulnerability functions: here we just trim the chain upon reaching the
sink in the vulnerability_helper.
@@ -56,16 +57,13 @@ def __init__(
self.sink = sink
self.sink_trigger_word = sink_trigger_word

self.reassignment_nodes = reassignment_nodes
self._remove_sink_from_secondary_nodes()
# Remove the sink node and all nodes after the sink from the list of reassignments.
Copy link
Collaborator

@KevinHock KevinHock Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️
This is amazing, it used to be more confusing but I forgot about after the sink. Thanks so much for making this.

self.reassignment_nodes = reassignment_nodes
self._remove_sink_from_secondary_nodes()
# Remove the sink node and all nodes after the sink from the list of reassignments.
self.reassignment_nodes = list(takewhile(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL takewhile

@KevinHock
Copy link
Collaborator

KevinHock commented Aug 16, 2018

I'm confused, shouldn't reassignment nodes always be the vulnerability chain (i.e. path from the source to the sink)? https://github.com/python-security/pyt/blob/master/pyt/vulnerabilities/vulnerabilities.py#L460 always gets hit, because I made --trim always true in https://github.com/python-security/pyt/blob/master/pyt/usage.py#L113-L118, but never cleaned up the UImode.NORMAL references e.g. https://github.com/python-security/pyt/blob/master/pyt/__main__.py#L68-L72 and https://github.com/python-security/pyt/blob/master/pyt/vulnerabilities/vulnerabilities.py#L459

I totally agree with you about the output.

@bcaller
Copy link
Collaborator Author

bcaller commented Aug 16, 2018

You're absolutely right. I'm stupid. The tests default to UImode.NORMAL but actually TRIM does what I want. Everything already works fine. 🤦‍♂️

@bcaller bcaller closed this Aug 16, 2018
@KevinHock
Copy link
Collaborator

You're definitely not stupid 🙂
I should still open and merge, it'll make it easier to clean up the UImode.NORMAL stuff later on :) The code is confusing because of it.

@KevinHock KevinHock reopened this Aug 16, 2018
@bcaller
Copy link
Collaborator Author

bcaller commented Aug 16, 2018

OK 😄

@KevinHock KevinHock merged commit d4d2e95 into python-security:master Aug 16, 2018
@bcaller bcaller deleted the shorter-reassignments branch September 7, 2018 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants